New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
replace Bluebird.timeout #3634
replace Bluebird.timeout #3634
Conversation
package.json
Outdated
@@ -39,6 +39,7 @@ | |||
"liftoff": "3.1.0", | |||
"lodash": "^4.17.15", | |||
"mkdirp": "^0.5.1", | |||
"p-timeout": "^3.2.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering how small p-yimeout is, maybe it would make sense to inline implementation and use native finally instead of p-finally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no strong opinion on that, though, sindresorhus libs are legit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Does we still support node 8? |
@maximelkin Yeah, ideally we want to support Node 8 for a bit longer. If that makes implementation complex, we can stick with p-timeout, it's not a big deal. |
This can make harder to move from bluebird completely, because Promise.finally only from 10 node |
@maximelkin doesn't p-timeout rely on p-finally which supports node 8? |
Yes, it is |
@maximelkin I'd much rather use p-finally directly or p-timeout rather than keep bluebird or break node 8 compatibility. |
p-timeout required finally because of support .cancel, we dont need it, so no troubles here |
ah, great :) |
@maximelkin Since #3639 is dropping support for Node 8 anyway, I guess this PR can do the same and we can merge both during next major release. |
Not directly related question: |
Knex will use only native promises and will not allow to choose the implementation. |
@kibertoad This pr actually not breaking node 8 compatibility, maybe we can merge it? |
@maximelkin There's a new unhandled Rejection on Node 8 happening ever since this was merged:
Wonder if there's a gap in our timeout implementation and if sindresorhus one would be more reliable. |
@kibertoad |
Breaking changes: TImeoutError now not extends Bluebird.TimeoutError
also this error exported explicitly